Repository navigation
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
📝 WalkthroughWalkthroughThe MCP tool generator now validates configured exclusions against query parameters and request-body fields. Wildcards can traverse array items and dictionary values. Tool definitions and generated schemas reflect updated exclusions. The change also adds feature-gated email-domain APIs and MCP tools, domain-wide MAIL FROM subdomain coordination, and a simulated evaluation workflow with setup guidance. Priority: ➖ Normal Merge Risk: 🔵 Low · up to The remaining issues are bounded. Require HTTPS for approval callbacks on HTTPS sites and correct the gated workflow guidance; the token message and DNS simulation also warrant follow-up. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Automated email-domain setup gains scoped access, while sender settings become shared across projects. Ownership checks and user approval constrain DNS changes, but HTTP return URLs remain accepted and mixed rollout can undermine domain-wide coordination. The affected scope is principally one sending domain and its related senders, not unrestricted access to other organizations. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
Full details: Description checkExplanation The description covers the MCP exclusion changes and their tests, but it omits substantial email-domain setup changes in the PR, including API and email integration behavior, gated MCP tools, evaluations, and workflow guidance. Its change table also does not match the broader file scope, and the release status says no feature flag controls the change despite the new tools being feature-flagged. Resolution Update Problem and Changes to cover the email-domain setup feature and its user-visible behavior, including the feature-flagged tools and shared MAIL FROM behavior. Correct the file and line counts, document tests for the email, API, and evaluation changes, and set the release status to match the feature flag. Add the required before-and-after Mermaid flowcharts for the workflow changes. Address the reported missing screenshot or demo if the frontend changes are part of this PR.
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
|
Risk: Low · 1 low Since the last review, this PR swaps the feature-flag 404 gate on the domain-connect apply-url endpoint for a scoped-credential check: proxy context is now flatly denied to token callers, email context stays flag-gated for tokens via EmailDomainAgentSetupPermission, and session users get flag-free access to both tenant-scoped resolution paths; the rest is removal of Sentinel reviewed |
👀 Auto-assigned reviewersThese soft owners were skipped because they only have minor changes here. Nothing blocks merge, so self-assign if you'd like a look:
Soft owners come from each directory's |
|
Thanks for taking the time to contribute to PostHog! ✅ All automated hygiene checks pass:
I've requested a review from the product-analytics team — they'll take a look shortly. |
| (resolved.operation.parameters ?? []).filter((p) => p.in === 'query').map((p) => p.name) | ||
| ) | ||
| const bodySchema = resolved.operation.requestBody?.content?.['application/json']?.schema | ||
| for (const entry of config.exclude_params ?? []) { |
There was a problem hiding this comment.
MCP agents can still pass inline credentials via external-data-sources-partial-update
The enabled external-data-sources-partial-update tool's input schema still exposes client_secret, account_id and job_inputs (see services/mcp/src/generated/warehouse_sources/api.ts:1744-1745 and :1772, confirmed in its tool-schema snapshot), so an agent can paste a secret into chat instead of routing the user through the connect-link flow the tool's own description prescribes. The PR removed the client_secret/account_id exclusions reasoning that only refresh and reload serializers carry them, but the partial-update tool shares that serializer and has no exclude_params entry at all (products/warehouse_sources/mcp/tools.yaml:498-506), and the new check iterates config.exclude_params ?? [] — an enabled tool with no entry is never validated, so this gap cannot fail the build.
How:
- Build passes on this branch: the partial-update tool has no
exclude_params, soassertExclusionsNameRealFieldsskips it entirely. - The tool's input schema (generated from the serializer body at
api.ts:1729-1773) includesclient_secret,account_id,job_inputs. - An agent rotating a credential sets
client_secretinline in the partial-update call, putting the secret in chat context — the exact outcome the sibling refresh/reload exclusions exist to prevent.
Fix: Add client_secret, account_id, job_inputs to external-data-sources-partial-update's exclude_params (this also demonstrates the gate should flag enabled write tools whose bodies carry credential-shaped fields with no entry).
React with 👍 if useful or 👎 if not
webjunkie
left a comment
There was a problem hiding this comment.
Looks good. The dict wildcard and the build check both hold up, and excludePath safely skips a missing or boolean additionalProperties.
Agent review. A person authorized the approval but did not review this text.
tomasfarias
left a comment
There was a problem hiding this comment.
Only partial review for batch export changes, so not approval. I imagine that changes in our product fall into this:
The check found 52 stale entries in 6 product YAML files
If so, I see no blockers 👍
ebd59c3 to
c44a531
Compare
|
Thanks for taking the time to contribute to PostHog! We get a lot of pull requests — to help get this PR reviewed faster, we have some checks in place to help you:
Push a change or update the description and we'll automatically re-review the PR. Once all checks are green, the team will be notified. |
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (4)
posthog/api/integration.py (2)
1474-1483: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRename the denial message. Session-only access does not explain the denial.
EmailDomainAgentSetupPermissiondenies every scoped token while the flag is off. That includes OAuth access tokens (MCP), not only personal API keys. An MCP caller receives "This action does not support personal API key access", which is wrong for that caller. Use a neutral message, for example "This action is not available for token access yet."Proposed fix
- message = "This action does not support personal API key access" + message = "This action is not available for API token access"Update the matching assertion in
posthog/api/test/test_integration.pyLine 2010.
2809-2814: 🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🟡 Minor | ⚡ Quick winWeak Cryptography
Reachability: External
Exploitability: Difficult
CWE: CWE-319 — Cleartext Transmission of Sensitive InformationRequire HTTPS for the callback when
SITE_URLis HTTPS. The validator accepts a same-hosthttp://URI and returns it unchanged. Domain Connect includes that URI in the signed apply URL and uses it to redirect the browser after the flow. Browsers that do not upgrade the host through HSTS can make the callback request over cleartext.Require HTTPS for HTTPS sites
- posthog_host = urlsplit(settings.SITE_URL).netloc - if redirect_uri and not url_has_allowed_host_and_scheme(redirect_uri, allowed_hosts={posthog_host}): + site = urlsplit(settings.SITE_URL) + if redirect_uri and not url_has_allowed_host_and_scheme( + redirect_uri, allowed_hosts={site.netloc}, require_https=site.scheme == "https" + ):products/workflows/evals/simulated_email_domains.py (1)
299-304: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn
NoAnswerwhen the name exists but lacks the requested record type.The
mail_from_label_takensetup publishes an MX record atfeedback.<domain>. If the simulator receives a lookup for another type at that name,_simulated_answersreports NXDOMAIN instead of NoAnswer. Check whether any record exists for the normalized name before choosing the exception.Suggested fix
def answers(self, name: str, record_type: str) -> set[str]: with self._lock: return set(self._records.get((_normalize(name), record_type), set())) + def has_name(self, name: str) -> bool: + normalized = _normalize(name) + with self._lock: + return any(record_name == normalized for record_name, _ in self._records) + @@ values = domains.answers(str(qname), record_type) if not values: + if domains.has_name(str(qname)): + raise dns.resolver.NoAnswer() raise dns.resolver.NXDOMAIN()products/workflows/skills/setting-up-an-email-domain/SKILL.md (1)
14-16: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep feature-gated tools out of globally shipped workflow guidance. These files ship to every MCP client, but they depend on email tools that are not available to every client.
products/workflows/skills/setting-up-an-email-domain/SKILL.md#L14-L16: remove the gated workflow from the global skill or make its required tools available to every client.products/workflows/skills/setting-up-an-email-domain/references/publishing-records.md#L3-L3: make this reference usable without the gated verifier or keep it with guidance available only when that tool is available.As per path instructions: “These skills ship to every MCP client. Flag any instruction that uses a command, flag, tool, or capability not available to every MCP user: flag-gated, client-specific, or mode-specific.”
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
0c598579-64ca-4d92-92ee-33682445dca0
⛔ Files ignored due to path filters (7)
products/integrations/frontend/generated/api.schemas.tsis excluded by!**/generated/**products/integrations/frontend/generated/api.tsis excluded by!**/generated/**products/integrations/frontend/generated/api.zod.tsis excluded by!**/generated/**services/mcp/src/generated/cdp_functions/api.tsis excluded by!**/generated/**services/mcp/src/generated/error_tracking_alerts/api.tsis excluded by!**/generated/**services/mcp/src/generated/integrations/api.tsis excluded by!**/generated/**services/mcp/src/tools/generated/integrations.tsis excluded by!**/generated/**
📒 Files selected for processing (52)
docs/published/handbook/engineering/ai/implementing-mcp-tools.mdposthog/api/integration.pyposthog/api/integration_domain_serializers.pyposthog/api/test/test_integration.pyposthog/api/test/test_integration_domain_serializers.pyposthog/domain_connect.pyposthog/models/integration/email.pyposthog/models/test/integration/test_email.pyposthog/test/test_domain_connect.pyproducts/autoresearch/mcp/tools.yamlproducts/batch_exports/mcp/tools.yamlproducts/billing/mcp/tools.yamlproducts/dashboards/mcp/tools.yamlproducts/integrations/mcp/tools.yamlproducts/persons/mcp/tools.yamlproducts/posthog_ai/eval_harness/harness/services.pyproducts/posthog_ai/eval_harness/test/test_sandboxed_harness_services.pyproducts/warehouse_sources/mcp/tools.yamlproducts/workflows/evals/__init__.pyproducts/workflows/evals/eval_email_domain_setup.pyproducts/workflows/evals/scorers.pyproducts/workflows/evals/seeders.pyproducts/workflows/evals/simulated_email_domains.pyproducts/workflows/evals/test/__init__.pyproducts/workflows/evals/test/test_scorers.pyproducts/workflows/evals/test/test_simulated_email_domains.pyproducts/workflows/package.jsonproducts/workflows/skills/setting-up-an-email-domain/SKILL.mdproducts/workflows/skills/setting-up-an-email-domain/references/publishing-records.mdservices/mcp/schema/generated-tool-definitions.jsonservices/mcp/schema/tool-definitions-all.jsonservices/mcp/schema/tool-inputs.jsonservices/mcp/scripts/generate-orval-schemas.mjsservices/mcp/scripts/generate-tools.tsservices/mcp/scripts/lib/definitions.mjsservices/mcp/src/api/generated.tsservices/mcp/src/schema/tool-inputs.tsservices/mcp/tests/unit/__snapshots__/tool-schemas/cdp-functions-create.jsonservices/mcp/tests/unit/__snapshots__/tool-schemas/cdp-functions-partial-update.jsonservices/mcp/tests/unit/__snapshots__/tool-schemas/error-tracking-alerts-create.jsonservices/mcp/tests/unit/__snapshots__/tool-schemas/error-tracking-alerts-partial-update.jsonservices/mcp/tests/unit/__snapshots__/tool-schemas/integrations-domain-connect-apply-url-create.jsonservices/mcp/tests/unit/__snapshots__/tool-schemas/integrations-domain-connect-check-retrieve.jsonservices/mcp/tests/unit/__snapshots__/tool-schemas/integrations-email-create.jsonservices/mcp/tests/unit/__snapshots__/tool-schemas/integrations-email-partial-update.jsonservices/mcp/tests/unit/__snapshots__/tool-schemas/integrations-email-verify-create.jsonservices/mcp/tests/unit/generate-tools.test.tsservices/mcp/tests/unit/integrations-domain-connect-apply-url.test.tsservices/mcp/tests/unit/tool-definitions.test.tsservices/mcp/tests/unit/tool-filtering.test.tstools/openapi-codegen/src/exclusions.mjstools/openapi-codegen/tests/exclusions.test.mjs
💤 Files with no reviewable changes (12)
- products/workflows/evals/init.py
- products/workflows/evals/test/init.py
- products/persons/mcp/tools.yaml
- services/mcp/tests/unit/snapshots/tool-schemas/error-tracking-alerts-partial-update.json
- products/warehouse_sources/mcp/tools.yaml
- services/mcp/tests/unit/snapshots/tool-schemas/cdp-functions-create.json
- products/dashboards/mcp/tools.yaml
- products/billing/mcp/tools.yaml
- services/mcp/tests/unit/snapshots/tool-schemas/error-tracking-alerts-create.json
- products/autoresearch/mcp/tools.yaml
- services/mcp/tests/unit/snapshots/tool-schemas/cdp-functions-partial-update.json
- products/batch_exports/mcp/tools.yaml
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
SES stores the MAIL FROM subdomain on the domain identity, so every sender on a domain shares it. Creating a sender with a different label moved it in SES for every sender on the domain, which only the admin-gated update route should do. Create now reuses the domain's label when the request omits it, and rejects a different label with a 400 before SES is called. Update writes the new label to every sender on the domain in the organization, so their stored config stays in step with SES. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A misspelled or stale `exclude_params` entry was a silent no-op, so the field it meant to hide stayed in the tool input. The generator now fails when an entry names no query parameter or request body path. The check found 51 entries that name read-only response fields or parameters the operation does not take. They hid nothing, so removing them leaves every generated tool schema unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
c44a531 to
fa800ed
Compare
|
Thanks for taking the time to contribute to PostHog! ✅ All automated hygiene checks pass:
I've requested a review from the product-analytics team — they'll take a look shortly. |
|
Note 🤖 GPT-6.1 Sol responding on behalf of Michael Replaced by the draft fork PR #346 in its native fork stack. @meikelmosby will start migrating the stack from the fork into PostHog/posthog for upstream review and CI. Closing this master-targeted copy to keep the handoff clear. |
|
Superseded by #115571, rebased on current master. |
Problem
cdp-functions-create,cdp-functions-partial-update,error-tracking-alerts-createanderror-tracking-alerts-partial-updateMCP tools can set four internal input fields:templating,order,bytecodeandtranspiled.inputs.*.templating), but the generated schema still exposes them.*segment in an exclusion path steps only into arrays.inputsis a dict, so the entry matches nothing.exclude_paramsentry that names no field, so a typo or a stale name also leaves the field exposed. parameterai flagged this on fix(mcp): reject conflicting exclusions on shared operations #111366.Warning
🟡 Merge risk: medium (merge-risk rubric)
templatingper input, so the backend falls back to the template's setting orhog.exclude_paramsentry that names no field. An open PR that adds such an entry failshogli build:openapiafter it rebases.Changes
templating,order,bytecodeortranspiledoninputs.*andmappings.*.inputs.*.*in an exclusion path now steps into dict values (additionalProperties) as well as array items.exclude_paramsentry names no query parameter or request body path. The error names the tool and the entry, like the other YAML gates.external_data_sources_createentries forclient_secret,account_idandjob_inputsgo too. That serializer takes credentials in a free-formpayloaddict, which no exclusion can reach. The refresh and reload tools still exclude these fields, because their serializer has them.How did you test this code?
Test rationale:
tools/openapi-codegen/tests/exclusions.test.mjscatches*failing to remove a field from dict values, at the top level and nested under an array. The closest existing test covers array items only.services/mcp/tests/unit/generate-tools.test.tscatches anexclude_paramsentry that names no field passing silently: a typo, a path parameter, or an inherited key such asconstructor. It also checks that valid entries pass, through arrays,oneOfand dicts. No earlier test covered entry validation.applyNestedExclusionsandgenerateCategoryFile, the same loop the other YAML gates test through. Deleting the check's call from that loop fails 8 cases.Run against an OpenAPI spec built from this branch with
hogli build:openapi-schemaflags:Generator output with
inputs.*.templatngincdp_functions.yamlhogli ci:preflight --strict --against upstream/masterpassed with no failures.👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Release status
Docs update
implementing-mcp-tools.mdnow describes the dict wildcard and the build failure for an entry that names no field.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Claude Opus 5.5 (
claude-opus-5-5)generate-tools.ts. A duplicate search for openexclude_paramsPRs found none. feat(mcp): report ignored input keys and cap tool schema growth #111786 reports ignored input keys at runtime, which is a separate change./qa-swarmrounds, GPT-6.1 Sol (correctness, risk, design) and Claude Opus 5.5 (fresh eyes). Round 1 found inherited keys and path parameters passing the check, and an error without the tool name. Round 2 moved the tests onto thegenerateCategoryFileseam. All four are fixed.inputs.*.bytecode_contractis read-only on the server but still appears in the hog function tool input./tdd,/merge-risk,/qa-swarm,/writing-pr-descriptions.🤖 Generated with Claude Code